Divyanshu | Design Improvements#515
Merged
Saurabh186 merged 3 commits intodevelopmentfrom Apr 1, 2026
Merged
Conversation
…ve service list styling - Create UiSettingsService to centralize theme and side nav state management with localStorage persistence and 30-day TTL - Create SideNavService to manage sidebar open/close state - Remove mat-card wrappers from service lists in create-feature and management components - Add service icons to management sections (Roles, Permissions, Snippet, Settings) - Implement getServiceListItems method to map services with dynamic icons based
…stency - Add h-full class to root container for proper height - Update webhook events track expression to use event.value - Increase gap spacing from gap-1 to gap-2 in Session & Token Settings and Webhook Configuration sections - Change Button Container Div icon from 'html' to 'code' - Fix indentation in preview template - Update logo URL placeholder to be more descriptive example
Saurabh186
approved these changes
Apr 1, 2026
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (12)
- apps/36-blocks/src/app/layout/layout.component.html (74-74) The `isSideNavOpen` input property for `proxy-main-left-side-nav` seems to have inverted logic. You're passing `[isSideNavOpen]="!sideNavService.isSideNavOpen()"` which means the property is `true` when the sidebar is actually closed. Consider renaming this property to `isCollapsed` or inverting the logic to make it more intuitive.
-
apps/36-blocks/src/app/layout/ui-settings.service.ts (45-58)
Consider adding a check for localStorage availability before attempting to use it. This will prevent errors in private browsing modes or when storage permissions are denied.
private _load(): UiSettings { if (!this._isLocalStorageAvailable()) { return { ...DEFAULTS, _savedAt: Date.now() }; } try { const raw = localStorage.getItem(STORAGE_KEY); if (raw) { const parsed: UiSettings = JSON.parse(raw); if (parsed._savedAt && Date.now() - parsed._savedAt < TTL_MS) { return { ...DEFAULTS, ...parsed }; } } } catch { // corrupted storage — fall through to defaults } return { ...DEFAULTS, _savedAt: Date.now() }; } private _isLocalStorageAvailable(): boolean { try { const test = 'test'; localStorage.setItem(test, test); localStorage.removeItem(test); return true; } catch { return false; } } -
apps/36-blocks/src/app/layout/ui-settings.service.ts (1-3)
Consider adding Observable streams for theme and sideNavOpen settings to allow components to reactively respond to changes using the Angular reactive pattern.
import { Injectable } from '@angular/core'; import { BehaviorSubject, Observable } from 'rxjs'; export type AppTheme = 'light-theme' | 'dark-theme'; - apps/36-blocks/src/app/features/create-feature/create-feature.component.html (344-344) The tracking expression has been changed from `track event` to `track event.value`. Make sure that `event.value` is unique and stable across renders to avoid potential performance issues with Angular's change detection. If `event.value` could be duplicated across different events, consider using a more unique identifier.
- apps/36-blocks/src/app/features/create-feature/create-feature.component.html (1860-1860) Good improvement on the logo URL input placeholder. Make sure that the example URL format (`https://example.com/logo.png`) matches the expected format for logo URLs in your application. Consider adding validation to ensure users enter properly formatted URLs.
- apps/36-blocks/src/app/features/create-feature/create-feature.component.ts (225-235) The `_serviceIcon` method uses string includes for icon mapping, which could lead to unexpected results if service names contain multiple matched keywords (e.g., a service named 'email_password' could match both 'email' and 'password' conditions). Consider using more specific matching logic or prioritizing the matches.
- libs/ui/service-list/src/lib/service-list.component.html (1-1) The change from `mat-list` to `mat-nav-list` is appropriate for navigation, but consider keeping the `role="list"` attribute to ensure maximum accessibility support across all screen readers.
- apps/36-blocks/src/app/users/management/management.component.html (34-34) The `w-full` class on the block selector will make it take up 100% width on all screen sizes, which might be excessive on larger screens. Consider using a more responsive approach like `w-xs-100 md:w-auto` or setting a maximum width.
- apps/36-blocks/src/app/users/management/management.component.html (50-55) Removing the mat-card wrapper around the sidebar navigation eliminates the visual container that helped distinguish the navigation from the content area. Consider adding some border or background styling to the proxy-service-list to maintain visual separation.
-
libs/ui/search/src/lib/search/search.component.html (1-1)
Replacing the semantic class name 'search-form-field' with a Tailwind utility class '!min-w-[280px]' might impact maintainability. Consider keeping the original class name alongside the Tailwind class for backward compatibility:
<mat-form-field class="search-form-field !min-w-[280px]" [class]="matFormFieldClass()?.length ? matFormFieldClass() : ''"> - libs/ui/service-list/src/lib/service-list.component.html (11-13) The current implementation always shows either an error icon or a navigate_next icon. Is this intentional? If some items shouldn't have navigation indicators, consider adding another condition to check if the item is navigable.
-
apps/36-blocks/src/app/layout/layout.component.html (33-35)
There's a syntax issue with the mat-icon element. The closing bracket and tag are improperly formatted.
<mat-icon class="mat-icon-22" [matTooltip]="clientSettings?.client?.name" matTooltipPosition="right">apps</mat-icon>
💡 To request another review, post a new comment with "/windsurf-review".
| private rootService = inject(RootService); | ||
| private authService = inject(AuthService); | ||
| private cdr = inject(ChangeDetectorRef); | ||
| public sideNavService = inject(SideNavService); |
There was a problem hiding this comment.
The sideNavService is injected as public but should be private since it's only used internally in component methods. This helps encapsulate implementation details.
Comment on lines
+38
to
+39
| import { SideNavService } from './side-nav.service'; | ||
| import { UiSettingsService } from './ui-settings.service'; |
There was a problem hiding this comment.
The SideNavService and UiSettingsService are imported but not included in the component's imports array. Make sure to add them to ensure proper dependency declaration.
| mat-stretch-tabs="false" | ||
| animationDuration="200ms" | ||
| class="h-full" | ||
| (selectedTabChange)="onTabChange($event.index)" |
There was a problem hiding this comment.
The PR adds a (selectedTabChange)="onTabChange($event.index)" event handler, but there's no corresponding implementation of this method visible in the diff. Please ensure the onTabChange method is properly implemented in the component class to avoid runtime errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.